Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Projections generated code may fail compilation due to order of switch case statements #2751

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

svrx
Copy link
Contributor

@svrx svrx commented Oct 13, 2023

After upgrade to version 6 some issue have been identified with compilation of generated code.
When a projection depends on events from different levels of the same inheritance chain, the sorting of methods performed during code generation has been found to be non-deterministic.

Example:

//Events
class BaseEvent {}
class DerivedEventA: BaseEvent {}
class DerivedEventB: BaseEvent {}

//Projection
class Aggregate{
   void Apply(DerivedEventA @event):
   void Apply(DerivedEventB @event):
   void Apply(BaseEvent @event):
}

Will sometimes throw a Unreachable code detected error during compilation of generated code, due to having BaseEvent appearing in switch case order before any of the derived events.

Further investigation has shown that root cause of the issue may reside in EventTypeComparer implementation.
At first glance it looks fine, although the resolution of comparison isn't granular enough, hence when being processed for sorting through .Net internals (depending on API flavour used, may be a QuickSort or Heap+Introspective sort), and optimizations are applied in regards divide and conquer nature of algorithms, having assumptions taken based on partitions and equality (as per result of comparer), which in turn results in some entries never being compared, since all the items in the partition being processed having been reported to be equal by the comparer (comparer returns 0).

Relates to #2749.

svrx added 4 commits October 11, 2023 15:20
…odegen and implements extensive sorting tests

Disclaimer: Comparer implementation may not work properly if interface types are used in projections.
Current implementation relies on using entire type hierarchy for sorting, starting from base type, within same hierarchy level type name sort is used.
…g logic in `EventTypePatternMatchFrame`

Adds support for having event types as interfaces of that is ever a valid scenario.
Adjust tests in accordance to functional changes.
@svrx
Copy link
Contributor Author

svrx commented Oct 16, 2023

The implementation was updated from prior version for simplicity and better encapsulation.
The sort method was encapsulated in EventTypePatternMatchFrame has that's where the logic is relevant and changed back to using simpler comparer originally used in combination with a different sort approach to avoids original issues.
All usages were updated accordingly.

@jeremydmiller jeremydmiller added this to the 6.3.0 milestone Oct 19, 2023
@svrx svrx changed the title Fix for #2749 - Projections generated code sorts switch cases in reliable manner Projections generated code may fail compilation due to order of switch case statements Oct 19, 2023
@jeremydmiller
Copy link
Member

I'm working on this one now

@jeremydmiller jeremydmiller self-assigned this Oct 19, 2023
@jeremydmiller
Copy link
Member

And #sadtrombone, this change breaks a bunch of tests. I'm investigating now.

@jeremydmiller jeremydmiller merged commit f8d80eb into JasperFx:master Oct 19, 2023
@jeremydmiller
Copy link
Member

@svrx Just for your later reference, the usage of SortedList had some unexpected side effects where it would lose several event handlers. I switched the code to just use List<T>.Sort(), and that did the trick.

@svrx
Copy link
Contributor Author

svrx commented Oct 19, 2023

@jeremydmiller you're right, apologies for overlooking that fact. In this case this PR is doing nothing. EventTypePatternMatchFrameOrderTests are failing the same way they would have before. Are these test run as part of your validation pipelines?
I can revert comparer to my interim implementation that didn't had that issue.
Question before I move with that, are interfaces that events may implement allowed to be used in projections? Or any plans to support something like that?

@svrx svrx deleted the codegen-apply-order-scenario branch October 20, 2023 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants